Open
Conversation
|
Firetiger deploy monitoring skipped This PR didn't match the auto-monitor filter configured on your GitHub connection:
Reason: PR modifies client libraries and session handling, not API endpoints (packages/api/cmd/api/) or Temporal workflows (packages/api/lib/temporal) as specified in the filter. To monitor this PR anyway, reply with |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed:
HTTPClient()never extracts user's custom HTTP client- Changed
option.WithHTTPClientto returnrequestconfig.PreRequestOptionFuncsoPreRequestOptionsnow applies it andBrowserSessionClient.HTTPClient()receives the caller’s custom client.
- Changed
Or push these changes by commenting:
@cursor push 3b2a924f02
Preview (3b2a924f02)
diff --git a/option/requestoption.go b/option/requestoption.go
--- a/option/requestoption.go
+++ b/option/requestoption.go
@@ -56,7 +56,7 @@
// For custom uses cases, it is recommended to provide an [*http.Client] with a custom
// [http.RoundTripper] as its transport, rather than directly implementing [HTTPClient].
func WithHTTPClient(client HTTPClient) RequestOption {
- return requestconfig.RequestOptionFunc(func(r *requestconfig.RequestConfig) error {
+ return requestconfig.PreRequestOptionFunc(func(r *requestconfig.RequestConfig) error {
if client == nil {
return fmt.Errorf("requestoption: custom http client cannot be nil")
}You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: SDK-level dependency on code-gen-only
golang.org/x/toolsforces Go version bump- I moved
internal/genbrowsersessionservicesto its own Go module and updated generation entrypoints sogolang.org/x/toolsno longer affects the SDK module, allowing the rootgo.modto stay at Go 1.22 without tool-only deps.
- I moved
Or push these changes by commenting:
@cursor push bac079904b
Preview (bac079904b)
diff --git a/browser_session.go b/browser_session.go
--- a/browser_session.go
+++ b/browser_session.go
@@ -1,6 +1,6 @@
package kernel
-//go:generate go run ./internal/genbrowsersessionservices -output browser_session_services_gen.go
+//go:generate sh -c "cd internal/genbrowsersessionservices && go run . -dir ../.. -output browser_session_services_gen.go"
import (
"fmt"
diff --git a/go.mod b/go.mod
--- a/go.mod
+++ b/go.mod
@@ -1,16 +1,13 @@
module github.com/kernel/kernel-go-sdk
-go 1.23.0
+go 1.22
require (
github.com/tidwall/gjson v1.18.0
github.com/tidwall/sjson v1.2.5
- golang.org/x/tools v0.31.0
)
require (
github.com/tidwall/match v1.1.1 // indirect
- github.com/tidwall/pretty v1.2.1 // indirect
- golang.org/x/mod v0.24.0 // indirect
- golang.org/x/sync v0.12.0 // indirect
+ github.com/tidwall/pretty v1.2.0 // indirect
)
diff --git a/go.sum b/go.sum
--- a/go.sum
+++ b/go.sum
@@ -1,18 +1,9 @@
-github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
-github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/tidwall/gjson v1.14.2/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
github.com/tidwall/gjson v1.18.0 h1:FIDeeyB800efLX89e5a8Y0BNH+LOngJyGrIWxG2FKQY=
github.com/tidwall/gjson v1.18.0/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA=
github.com/tidwall/match v1.1.1/go.mod h1:eRSPERbgtNPcGhD8UCthc6PmLEQXEWd3PRB5JTxsfmM=
+github.com/tidwall/pretty v1.2.0 h1:RWIZEg2iJ8/g6fDDYzMpobmaoGh5OLl4AXtGUGPcqCs=
github.com/tidwall/pretty v1.2.0/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
-github.com/tidwall/pretty v1.2.1 h1:qjsOFOWWQl+N3RsoF5/ssm1pHmJJwhjlSbZ51I6wMl4=
-github.com/tidwall/pretty v1.2.1/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY=
github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28=
-golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU=
-golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww=
-golang.org/x/sync v0.12.0 h1:MHc5BpPuC30uJk597Ri8TV3CNZcTLu6B6z4lJy+g6Jw=
-golang.org/x/sync v0.12.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
-golang.org/x/tools v0.31.0 h1:0EedkvKDbh+qistFTd0Bcwe/YLh4vHwWEkiI0toFIBU=
-golang.org/x/tools v0.31.0/go.mod h1:naFTU+Cev749tSJRXJlna0T3WxKvb1kWEx15xA4SdmQ=
diff --git a/internal/genbrowsersessionservices/go.mod b/internal/genbrowsersessionservices/go.mod
new file mode 100644
--- /dev/null
+++ b/internal/genbrowsersessionservices/go.mod
@@ -1,0 +1,10 @@
+module github.com/kernel/kernel-go-sdk/internal/genbrowsersessionservices
+
+go 1.23.0
+
+require golang.org/x/tools v0.31.0
+
+require (
+ golang.org/x/mod v0.24.0 // indirect
+ golang.org/x/sync v0.12.0 // indirect
+)
diff --git a/internal/genbrowsersessionservices/go.sum b/internal/genbrowsersessionservices/go.sum
new file mode 100644
--- /dev/null
+++ b/internal/genbrowsersessionservices/go.sum
@@ -1,0 +1,8 @@
+github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU=
+golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww=
+golang.org/x/sync v0.12.0 h1:MHc5BpPuC30uJk597Ri8TV3CNZcTLu6B6z4lJy+g6Jw=
+golang.org/x/sync v0.12.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
+golang.org/x/tools v0.31.0 h1:0EedkvKDbh+qistFTd0Bcwe/YLh4vHwWEkiI0toFIBU=
+golang.org/x/tools v0.31.0/go.mod h1:naFTU+Cev749tSJRXJlna0T3WxKvb1kWEx15xA4SdmQ=
diff --git a/scripts/generate-browser-session b/scripts/generate-browser-session
--- a/scripts/generate-browser-session
+++ b/scripts/generate-browser-session
@@ -4,4 +4,7 @@
cd "$(dirname "$0")/.."
-go run ./internal/genbrowsersessionservices -output browser_session_services_gen.go
+(
+ cd internal/genbrowsersessionservices
+ go run . -dir ../.. -output browser_session_services_gen.go
+)You can send follow-ups to the cloud agent here.
Bind browser subresource calls to a browser session's base_url and expose raw HTTP through a standard http.Client so metro-routed access feels like normal Go networking. Made-with: Cursor
Use the browser session base_url directly for path rewriting, preserve custom HTTP clients in HTTPClient(), and add an env-gated integration test for browser-scoped routing. Made-with: Cursor
Avoid depending on base_url path details in the integration test, keep the JWT helper package-private, and make round-tripper conformance explicit while preserving browser-scoped routing behavior. Made-with: Cursor
Keep the raw round-tripper constructor package-private, remove defensive middleware branches that imply unsupported empty inputs, and retain the browser-scoped integration coverage without baking in base_url path details. Made-with: Cursor
Replace the handwritten browser-scoped Go service façade with deterministic generated bindings derived from the generated browser service graph, and enforce regeneration in lint. Made-with: Cursor
rgarcia
force-pushed
the
raf/browser-scoped-client
branch
from
1720a28 to
b6a77bc
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Middleware path rewrite ignores URL RawPath field
- After rewriting req.URL.Path, the middleware now clears req.URL.RawPath so escaped-path requests consistently use the updated path, and a regression test covers this case.
Or push these changes by commenting:
@cursor push 4794dcee9b
Preview (4794dcee9b)
diff --git a/lib/browserscope/middleware.go b/lib/browserscope/middleware.go
--- a/lib/browserscope/middleware.go
+++ b/lib/browserscope/middleware.go
@@ -33,6 +33,7 @@
rest = "/" + rest
}
req.URL.Path = prefix + rest
+ req.URL.RawPath = ""
}
}
diff --git a/lib/browserscope/middleware_test.go b/lib/browserscope/middleware_test.go
--- a/lib/browserscope/middleware_test.go
+++ b/lib/browserscope/middleware_test.go
@@ -55,3 +55,27 @@
func TestBrowserSessionMiddlewareType(t *testing.T) {
var _ option.Middleware = BrowserSessionMiddleware("a", "b")
}
+
+func TestBrowserSessionMiddlewareClearsRawPathOnRewrite(t *testing.T) {
+ mw := BrowserSessionMiddleware("sess1", "")
+ var final *http.Request
+ next := func(req *http.Request) (*http.Response, error) {
+ final = req
+ return nil, nil
+ }
+
+ u, err := url.Parse("https://host/browser/kernel/browsers/sess1/process/%20exec")
+ if err != nil {
+ t.Fatal(err)
+ }
+ req := &http.Request{URL: u}
+
+ _, _ = mw(req, next)
+
+ if final.URL.RawPath != "" {
+ t.Fatalf("raw path should be cleared after rewrite: got %q", final.URL.RawPath)
+ }
+ if got := final.URL.EscapedPath(); got != "/browser/kernel/process/%20exec" {
+ t.Fatalf("escaped path rewrite: got %q", got)
+ }
+}You can send follow-ups to the cloud agent here.
Show the browser-scoped HTTPClient flow explicitly so the /curl/raw-backed public API is discoverable from a runnable Go example. Made-with: Cursor
Route browser subresources and raw HTTP through the shared browser route cache so the SDK no longer needs the generated browser-scoped client surface. Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Error from
PreRequestOptionssilently swallowed inHTTPClientBrowserService.HTTPClientnow returnsnil, errwhenrequestconfig.PreRequestOptionsfails, and a regression test verifies the error is propagated.
- ✅ Fixed: NewClient mutates shared option causing cross-client cache corruption
NewClientnow clones eachbrowserRoutingOptionbefore injecting the per-client cache so reused options no longer share mutable cache pointers, with a test confirming per-client cache isolation.
Or push these changes by commenting:
@cursor push 2f05d40ea0
Preview (2f05d40ea0)
diff --git a/browser.go b/browser.go
--- a/browser.go
+++ b/browser.go
@@ -174,7 +174,7 @@
}
cfg, err := requestconfig.PreRequestOptions(opts...)
if err != nil {
- return browserscope.HTTPClient(route.BaseURL, route.JWT, nil), nil
+ return nil, err
}
return browserscope.HTTPClient(route.BaseURL, route.JWT, cfg.HTTPClient), nil
}
diff --git a/browser_routing_test.go b/browser_routing_test.go
--- a/browser_routing_test.go
+++ b/browser_routing_test.go
@@ -111,3 +111,17 @@
t.Fatalf("expected control-plane path, got %q", got)
}
}
+
+func TestBrowserRoutingOptionReuseKeepsPerClientRouteCache(t *testing.T) {
+ routingOpt := WithBrowserRouting(BrowserRoutingConfig{Enabled: true, DirectToVMSubresources: []string{"process"}})
+
+ client1 := NewClient(routingOpt)
+ client2 := NewClient(routingOpt)
+
+ if got := browserRouteCacheFromOptions(client1.Options); got != client1.BrowserRouteCache {
+ t.Fatalf("expected client1 options to resolve client1 cache, got %p want %p", got, client1.BrowserRouteCache)
+ }
+ if got := browserRouteCacheFromOptions(client2.Options); got != client2.BrowserRouteCache {
+ t.Fatalf("expected client2 options to resolve client2 cache, got %p want %p", got, client2.BrowserRouteCache)
+ }
+}
diff --git a/browser_session_httpclient_test.go b/browser_session_httpclient_test.go
--- a/browser_session_httpclient_test.go
+++ b/browser_session_httpclient_test.go
@@ -1,11 +1,13 @@
package kernel
import (
+ "errors"
"io"
"net/http"
"net/http/httptest"
"testing"
+ "github.com/kernel/kernel-go-sdk/internal/requestconfig"
"github.com/kernel/kernel-go-sdk/lib/browserscope"
"github.com/kernel/kernel-go-sdk/option"
)
@@ -75,3 +77,30 @@
t.Fatal("expected cached route lookup failure")
}
}
+
+func TestBrowserSessionHTTPClientPropagatesPreRequestOptionError(t *testing.T) {
+ c := NewClient(
+ option.WithBaseURL("https://api.example/"),
+ option.WithAPIKey("sk"),
+ )
+
+ primeBrowserRouteCache(c.Options, browserscope.Ref{
+ SessionID: "sid",
+ BaseURL: "https://browser-session.test/browser/kernel",
+ CdpWsURL: "wss://x/browser/cdp?jwt=j1",
+ })
+
+ wantErr := errors.New("pre-request failed")
+ client, err := c.Browsers.HTTPClient(
+ "sid",
+ requestconfig.PreRequestOptionFunc(func(*requestconfig.RequestConfig) error {
+ return wantErr
+ }),
+ )
+ if !errors.Is(err, wantErr) {
+ t.Fatalf("expected error %q, got %v", wantErr, err)
+ }
+ if client != nil {
+ t.Fatal("expected nil client when pre-request options fail")
+ }
+}
diff --git a/client.go b/client.go
--- a/client.go
+++ b/client.go
@@ -65,9 +65,11 @@
func NewClient(opts ...option.RequestOption) (r Client) {
opts = append(DefaultClientOptions(), opts...)
cache := browserscope.NewRouteCache()
- for _, opt := range opts {
+ for i, opt := range opts {
if routing, ok := opt.(*browserRoutingOption); ok {
- routing.cache = cache
+ routingWithCache := *routing
+ routingWithCache.cache = cache
+ opts[i] = &routingWithCache
}
}
opts = append(opts, withBrowserRouteCache(cache))You can send follow-ups to the cloud agent here.
Drop the extra cache priming helper, remove metro wording, and rename the example so the go diff stays focused on direct-to-VM routing. Made-with: Cursor
rgarcia
changed the title
Rename the browser routing allowlist field to Subresources so the direct-to-VM configuration is shorter and easier to read. Made-with: Cursor
Rename the handwritten routing helpers to browserrouting, fix the shared-cache and RawPath issues, and revert the generated/module churn that should not stay in the PR. Made-with: Cursor
Drop the superseded lib/browserscope files now that the renamed browserrouting package owns the direct-to-VM helpers. Made-with: Cursor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Config error silently swallowed, dropping custom HTTP client
- The browser HTTP client constructor now returns the
NewRequestConfigerror instead of silently falling back to a default client.
- The browser HTTP client constructor now returns the
Or push these changes by commenting:
@cursor push 8a33f5475d
Preview (8a33f5475d)
diff --git a/browser_http_client.go b/browser_http_client.go
--- a/browser_http_client.go
+++ b/browser_http_client.go
@@ -27,7 +27,7 @@
cfg, err := requestconfig.NewRequestConfig(context.Background(), http.MethodGet, "https://example.com", nil, nil, opts...)
if err != nil {
- return browserrouting.NewHTTPClient(route.BaseURL, route.JWT, nil), nil
+ return nil, err
}
return browserrouting.NewHTTPClient(route.BaseURL, route.JWT, cfg.HTTPClient), nilYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit d594f39. Configure here.
Stop exposing browser routing rollout controls on the client constructor and derive direct-to-VM subresource routing from KERNEL_BROWSER_ROUTING_SUBRESOURCES instead, defaulting to curl while keeping raw HTTP helpers cache-backed.
Return request config errors from Browsers.HTTPClient instead of silently falling back to the default client, so invalid options do not drop custom HTTP behavior without notice. Add a regression test for the failure path. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
KERNEL_BROWSER_ROUTING_SUBRESOURCEScurlwhen the env var is unset, and treat an explicit empty string as fully disabling browser subresource routingBrowserRouteCache, which still powersBrowsers.HTTPClient(sessionID)and cache-backed direct/curl/rawrequestsNewClient()so the SDK can grow toward automatic routing without exposing rollout knobs in the public APIRollout behavior
curlsubresources directly to the browser VM"": disable browser subresource routing entirelyBrowsers.HTTPClient(sessionID)still always goes direct to the browser VM because it uses the cached browser route and/curl/rawTest plan
go test ./...KERNEL_API_KEY=... KERNEL_BASE_URL=https://api.onkernel.com go run ./examples/browser-routingNote
Medium Risk
Introduces automatic request rewriting middleware and a shared in-memory route cache that changes how some browser subresource calls are sent (direct to VM vs control plane), which could affect request routing and auth/header handling if misconfigured.
Overview
Adds direct-to-VM browser subresource routing driven by
KERNEL_BROWSER_ROUTING_SUBRESOURCES(defaults to routing onlycurlwhen unset; empty string disables routing), implemented as a middleware injected byNewClient().Introduces a shared
BrowserRouteCacheonClientthat is warmed automatically fromBrowsers.New/Get/Update/Listresponses and cleared onBrowsers.DeleteByID, and uses it to powerBrowsers.HTTPClient(sessionID)for browser-egress HTTP via{base_url}/curl/raw?jwt=....Adds the new
lib/browserroutingpackage (route cache, request rewrite middleware, raw curl round-tripper, jwt extraction), plus unit/integration tests and an example demonstrating routed requests.Reviewed by Cursor Bugbot for commit 1ec1358. Bugbot is set up for automated code reviews on this repo. Configure here.